chore: remove GetHeight from ClientMessage interface #1285

damiannolan · 2022-04-21T16:04:49Z

Description

Removing the GetHeight interface method from ClientMessage.
Returning a list of heights []exported.Height from the new UpdateState interface.
Enforce checking len(consensusHeights) to guard against index out of range errors on no-op updates
Adding new event key AttributeKeyConsensusHeights and returning a comma separated list of consensus heights

closes: #594

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

… updating 02-client events emission

modules/core/02-client/keeper/events.go

modules/core/02-client/types/events.go

modules/core/02-client/keeper/client.go

colin-axner

LGTM! My only concern is around handling duplicate updates. Probably best to just decide on our ideal solution and then see how it breaks relayers (and then revert if necessary)

modules/core/exported/client.go

modules/light-clients/06-solomachine/types/update.go

modules/light-clients/06-solomachine/types/update_test.go

modules/light-clients/07-tendermint/types/update.go

modules/core/02-client/types/events.go

modules/core/02-client/keeper/events.go

colin-axner · 2022-04-25T10:57:21Z

modules/core/02-client/keeper/events.go

 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
 			types.EventTypeUpdateClient,
 			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
 			sdk.NewAttribute(types.AttributeKeyClientType, clientType),
-			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
+			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeights[0].String()),


hmm, I think we need a len(consensusHeights) > 0 check

This was being checked in the UpdateClient() method of keeper.go.

if len(consensusHeights) != 0 { // emit events, otherwise no-op & return nil } return nil

ah yes. The only strange part is that this function assumes you pass in a non empty array of consensus heights

Unrelated, could we add // deprecated next to the consensus height attribute

Should we backport consensusHeights attribute to older releases? That way relayers could switch to it without waiting for all chains to upgrade to this version

The only strange part is that this function assumes you pass in a non empty array of consensus heights

Agree, it is kind of strange. I'll adapt to check len(consensusHeights) inside event emission func and remove the check from the UpdateClient keeper method.

Should we backport consensusHeights attribute to older releases?

Sounds like a good idea, but this would require a separate PR to main, correct?

Edit: I'll also add deprecated comment in-line 👍 Now that I'm looking at this once again, AttributeKeyConsensusHeight is used for create and upgrade events as a single height value, and should probably be maintained. In which case, I think the deprecation notice should only be here, solely for the update events and not in types/events.go

good catch!

colin-axner · 2022-04-25T10:59:01Z

modules/core/02-client/keeper/client.go


-	k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
+	if len(consensusHeights) != 0 {


I think not emitting UpdateClientEvent on a successful MsgUpdateClient is problematic. I may be wrong though. Would be good to check with relayers what the best expected functionality is for dup updates

The main odd thing is if you query for UpdateClient events, this message wouldn't come up. To me, it'd make more sense if the event was emitted, but indicated it was a duplicate update

I'm happy to change this to return the height from the ClientState implementation on duplicate updates.
However, I don't think we can provide information about duplicate updates in the events, that would only be possible if emitting from 07-tendermint for example, but as we discussed before they should be emitted from 02-client in a somewhat standardised fashion.

If we're in agreement, I can update this PR to return the height on duplicates and also make the changes to remove the error return value and panic instead.

Wdyt?

I was thinking if consensus_heights is empty, we should just return "" in the consensus heights attribute. Thoughts?

In favor of removing error return 👍

I updated and removed the error return, and also returning the height on duplicate header updates from 07!

…ing GetHeight from solomachine Header, updating tests

… header update

…t-height

colin-axner

LGTM. I still have some reservations about not emitting events for successful MsgUpdateClient which the light client does not return consensus heights for. Naturally, it makes more sense to me to emit event which indicates that result (since otherwise if you query via events, this message cannot be retrieved).

colin-axner · 2022-04-26T11:03:00Z

modules/core/02-client/keeper/events.go

 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
 			types.EventTypeUpdateClient,
 			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
 			sdk.NewAttribute(types.AttributeKeyClientType, clientType),
-			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
+			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeights[0].String()),


ah yes. The only strange part is that this function assumes you pass in a non empty array of consensus heights

Unrelated, could we add // deprecated next to the consensus height attribute

colin-axner · 2022-04-26T11:03:31Z

modules/core/02-client/keeper/events.go

 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
 			types.EventTypeUpdateClient,
 			sdk.NewAttribute(types.AttributeKeyClientID, clientID),
 			sdk.NewAttribute(types.AttributeKeyClientType, clientType),
-			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeight.String()),
+			sdk.NewAttribute(types.AttributeKeyConsensusHeight, consensusHeights[0].String()),


Should we backport consensusHeights attribute to older releases? That way relayers could switch to it without waiting for all chains to upgrade to this version

colin-axner

Excellent! Thanks for following up on my suggestion. I think the previous solution was a great one, but I think this makes more sense given the history of how the SDK is used

damiannolan · 2022-04-27T13:10:15Z

Excellent! Thanks for following up on my suggestion. I think the previous solution was a great one, but I think this makes more sense given the history of how the SDK is used

Totally agree, I think we got to the best possible solution anyways! Thanks for the discussions and reviews 👍

* adding new consensus heights return value from UpdateState interface, updating 02-client events emission * removing GetHeight() from ClientMessage interface, updating tests * updating godocs to include returned consensus height data * removing unused event emission func, adding deprecation notice, removing GetHeight from solomachine Header, updating tests * removing error return from UpdateState, returning height on duplicate header update * updating event emissions as per suggestions

damiannolan added 3 commits April 21, 2022 17:36

adding new consensus heights return value from UpdateState interface,…

d72c9e1

… updating 02-client events emission

removing GetHeight() from ClientMessage interface, updating tests

9737a7e

updating godocs to include returned consensus height data

b30380f

damiannolan commented Apr 22, 2022

View reviewed changes

modules/core/02-client/keeper/events.go Outdated Show resolved Hide resolved

damiannolan commented Apr 22, 2022

View reviewed changes

modules/core/02-client/types/events.go Show resolved Hide resolved

damiannolan commented Apr 22, 2022

View reviewed changes

modules/core/02-client/keeper/client.go Show resolved Hide resolved

damiannolan marked this pull request as ready for review April 22, 2022 15:36

damiannolan requested review from colin-axner, fedekunze and AdityaSripal as code owners April 22, 2022 15:36

damiannolan requested review from charleenfei, crodriguezvega and seantking April 22, 2022 15:37

colin-axner reviewed Apr 25, 2022

View reviewed changes

damiannolan added 3 commits April 25, 2022 14:23

removing unused event emission func, adding deprecation notice, remov…

832ca2e

…ing GetHeight from solomachine Header, updating tests

removing error return from UpdateState, returning height on duplicate…

9116fd1

… header update

Merge branch '02-client-refactor' into damian/594-remove-clientmsg-ge…

2066ab9

…t-height

colin-axner reviewed Apr 26, 2022

View reviewed changes

updating event emissions as per suggestions

aca9870

seantking approved these changes Apr 26, 2022

View reviewed changes

colin-axner approved these changes Apr 27, 2022

View reviewed changes

damiannolan merged commit cf893c2 into 02-client-refactor Apr 27, 2022

damiannolan deleted the damian/594-remove-clientmsg-get-height branch April 27, 2022 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove GetHeight from ClientMessage interface #1285

chore: remove GetHeight from ClientMessage interface #1285

damiannolan commented Apr 21, 2022 •

edited

Loading

colin-axner left a comment

colin-axner Apr 25, 2022

damiannolan Apr 25, 2022

colin-axner Apr 26, 2022

colin-axner Apr 26, 2022

damiannolan Apr 26, 2022 •

edited

Loading

colin-axner Apr 27, 2022

colin-axner Apr 25, 2022

damiannolan Apr 25, 2022

colin-axner Apr 25, 2022

damiannolan Apr 26, 2022

colin-axner left a comment

colin-axner Apr 26, 2022

colin-axner Apr 26, 2022

colin-axner left a comment

damiannolan commented Apr 27, 2022


		k.Logger(ctx).Info("client state updated", "client-id", clientID, "height", consensusHeight.String())
		if len(consensusHeights) != 0 {

chore: remove GetHeight from ClientMessage interface #1285

chore: remove GetHeight from ClientMessage interface #1285

Conversation

damiannolan commented Apr 21, 2022 • edited Loading

Description

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan commented Apr 27, 2022

damiannolan commented Apr 21, 2022 •

edited

Loading

damiannolan Apr 26, 2022 •

edited

Loading